Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add structures::gdt::Entry type #380

Merged
merged 4 commits into from
Apr 16, 2022
Merged

Add structures::gdt::Entry type #380

merged 4 commits into from
Apr 16, 2022

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Apr 15, 2022

The current documentation for the GDT often confuses entries and Descriptors.
For example, adding a new Descriptor uses a method confusingly named
add_entry.

An entry is a raw 64-bit value that is indexed by a segment selector.
The MAX length of the GDT is a number of entries, not Descriptors.

To fix this confusion, this PR makes the following changes:

  • Adds a transparent u64 newtype called Entry.
  • Updates the GlobalDescriptorTable documentation to correctly use
    Entry or Descriptor where appropriate.
  • Renames the add_entry to append.
    • This better expresses that this method might add multiple entries.
  • Renames from_raw_slice to from_raw_entries
    • As @Freax13 provided out, if we add some checks, this method can be safe.
  • Renames as_raw_slice to entries
    • This method now returns a slice of Entrys instead of u64s

This also fixes an issue where our assert!s in empty() wouldn't
trigger if the GDT was constructed from raw values.

Signed-off-by: Joe Richey [email protected]

The current documentation for the GDT often confuses entries and `Descriptor`s.
For example, adding a new `Descriptor` uses a method confusingly named
`add_entry`.

An entry is a raw 64-bit value that is indexed by a segment selector.
The `MAX` length of the GDT is a number of _entries_, not `Descriptor`s.

To fix this confusion, this PR makes the following changes:
  - Adds a transparent `u64` newtype called `Entry`.
  - Updates the `GlobalDescriptorTable` documentation to correctly use
    `Entry` or `Descriptor` where appropriate.
  - Renames the `add_entry` to `append`.
    - This better expresses that this method might add multiple entries.
  - Renames `from_raw_slice` to `from_raw_entries`
  - Renames `as_raw_slice` to `entries`
    - This method now returns a slice of `Entry`s instead of `u64`s

This also fixes an issue where our `assert!`s in `empty()` wouldn't
trigger if the GDT was constructed from raw values.

Signed-off-by: Joe Richey <[email protected]>
Copy link
Member

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall changes seem good to me, I left some minor comments.

src/structures/gdt.rs Outdated Show resolved Hide resolved
src/structures/gdt.rs Outdated Show resolved Hide resolved
src/structures/gdt.rs Outdated Show resolved Hide resolved
Also update the documentation

Signed-off-by: Joe Richey <[email protected]>
src/structures/gdt.rs Show resolved Hide resolved
@josephlr josephlr requested a review from Freax13 April 15, 2022 18:28
src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Joe Richey <[email protected]>
Copy link
Member

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@josephlr
Copy link
Contributor Author

I'll wait for @phil-opp to look at this and #381 before merging (as it is a notable API change).

@josephlr josephlr mentioned this pull request Apr 15, 2022
13 tasks
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

src/structures/gdt.rs Show resolved Hide resolved
@josephlr josephlr merged commit e70b8a3 into next Apr 16, 2022
@josephlr josephlr deleted the gdt_entry branch April 16, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants